Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust XML output #1759

Conversation

sebrandon1
Copy link
Member

@sebrandon1 sebrandon1 commented Dec 20, 2023

Follow up to #1753

Changes:

  • Adds omitempty to all XML attr's.
  • Parse the startTime and endTime stamps correctly and place the duration in the XML.
  • Set FailureMessage and SkippedMessage structs to pointers so they can be nil if not used.

Additional:

  • Sort XML output alphabetically.

@sebrandon1 sebrandon1 self-assigned this Dec 20, 2023
@sebrandon1 sebrandon1 force-pushed the xml_modification branch 2 times, most recently from 7e4171a to 35fd838 Compare December 20, 2023 21:31
Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the XML file is now omitting the skipped and failure labels correctly, but now, all the tnf tests are reported by passed in the DCI UI 😅 https://www.distributed-ci.io/jobs/f848c42c-50a5-4f5d-8a8c-ce77080cb20e/tests/bb0b560a-7edd-4733-945d-6846c461fc4a

I've been taking a look to the Excel file in https://www.distributed-ci.io/jobs/f848c42c-50a5-4f5d-8a8c-ce77080cb20e/files > cnf-certification-tests_junit.xml, and I could see it's used Skipped and Failure in upper case. I think they should be in lower case, so skipped and failure.

DCI is not parsing these elements in fact. I've checked, e.g., a test that is failing, such as affiliated-certification-container-is-certified-digest, and the message provided under the Failure element is not displayed. I think that, by just changing that to lower case, and also for Skipped, it should be fine.

Also, I just wanted to emphasize what I commented yesterday regarding other aspects to check, that are still present:

  • Try to sort the tests in a specific way (alphabetically, in the order they're executed, etc.). Before, they were sorted based on the execution order, but now, they appear randomly, and it's a bit difficult to check the results from the same test suite. Having that in alphabetically order could be fine.
  • The execution time still looks like artificial, they're mostly 0.00 in all cases, but I believe the real execution time should be there.
  • The messages displayed on the skipped tests, they still seem to be less intuitive than the previous format. Is it feasible to return to the previous format?

@sebrandon1
Copy link
Member Author

The messages displayed for skipped cases is different than the previous branch and would have to be addressed seperately. @greyerof maybe we need to add more context there?

@greyerof
Copy link
Contributor

@sebrandon1 I'll take a look.

@sebrandon1 sebrandon1 force-pushed the xml_modification branch 3 times, most recently from 7124e98 to f0d0f79 Compare December 21, 2023 17:28
@sebrandon1
Copy link
Member Author

sebrandon1 commented Dec 21, 2023

@ramperher Please check my latest commit.

Changes:

  • Corrected the timestamping (hopefully). 😄
  • Sorted the output alphabetically.
  • Adjusted the Skipped and Failure to be lowercase now.

Copy link
Collaborator

@ramperher ramperher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it looks much better, thanks @sebrandon1 ! From my side, it's fine to merge it, the remaining aspect is to check the skipped message, but I think this can be done in a separate change.

@greyerof greyerof merged commit 6776ef9 into redhat-best-practices-for-k8s:ginkgo_removal Dec 22, 2023
16 checks passed
@sebrandon1 sebrandon1 deleted the xml_modification branch February 5, 2024 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants